Skip to content

Variations: Stop generation if there are more than 100 variations to generate #8541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jan 4, 2023
Merged

Variations: Stop generation if there are more than 100 variations to generate #8541

merged 8 commits into from
Jan 4, 2023

Conversation

@Ecarrion
Copy link
Contributor

@Ecarrion Ecarrion commented Jan 4, 2023

closes: #8489

Why

This PR makes sure to stop the generation process and inform the merchant when there is more than 100 generations to create.

How

  • Adde a new GenerationError type with a tooManyVariations case to represent the situation where there are more than 100 variations to create.

  • Integrate the error defined above in the ViewModel and in the ViewController to properly inform the merchant via a notice.

Demo

generation-error.mov

Testing Steps

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@Ecarrion Ecarrion added the feature: variation list Related to the variations list for variable products. label Jan 4, 2023
@Ecarrion Ecarrion added this to the 11.8 milestone Jan 4, 2023
@Ecarrion Ecarrion requested a review from ealeksandrov January 4, 2023 03:13
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 4, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8541-5636c23 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@Ecarrion Ecarrion linked an issue Jan 4, 2023 that may be closed by this pull request
Variations: Abort if there is more than 100 local variations #8489
Closed
ealeksandrov
ealeksandrov approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@ealeksandrov ealeksandrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loog good! Added 1 non-blocking improvement suggestion.

WooCommerce/Classes/ViewRelated/Products/Variations/ProductVariationsViewModel.swift
let variationsToGenerate = ProductVariationGenerator.generateVariations(for: product, excluding: existingVariations)
print("Variations to Generate: \(variationsToGenerate.count)")

// Guard for 100 variation limit
Copy link
Contributor

@ealeksandrov ealeksandrov Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we guard for this overflow early? So we won't actually create a lot of objects before failing? We should be able to quickly count possible combinations by multiplying the amount of options for each attribute.

But I'm not sure if we can easily subtract a number of existing variations - there may be duplicated combinations. Can we subtract just total number of existing variations for pre-generation guard (for a worst case)? And if it <100, we can proceed to existing check to make additional verification of combinations.

Copy link
Contributor Author

@Ecarrion Ecarrion Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we will hit a performance problem here right now. The plan is to eventually ask core to generate variations so probably not worth spending extra time on performance unless we notice something significant!

Base automatically changed from issue/8488-product-variation-generator to trunk January 4, 2023 20:49
@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 4, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@Ecarrion Ecarrion force-pushed the issue/8489-abort-if-multiple-variations branch from bd7482e to 150782a Compare January 4, 2023 20:54
@Ecarrion Ecarrion enabled auto-merge January 4, 2023 21:16
@Ecarrion Ecarrion merged commit d5b81b1 into trunk Jan 4, 2023
@Ecarrion Ecarrion deleted the issue/8489-abort-if-multiple-variations branch January 4, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ealeksandrov ealeksandrov ealeksandrov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature: variation list Related to the variations list for variable products.

Projects

None yet

Milestone

11.8

Development

Successfully merging this pull request may close these issues.

Variations: Abort if there is more than 100 local variations

4 participants

@Ecarrion @wpmobilebot @ealeksandrov